Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/api unknown path return error 2190 #2382

Merged

Conversation

DataDavD
Copy link
Contributor

@DataDavD DataDavD commented Aug 17, 2021

This PR fixes Issue #2190 by creating a handler to return invalid error for routes downstream of the BaseURL.

This PR fixes the issue by creating a handler that ensures that only routes on the mounted route (i.e. /api/v1) will be successful. Other routes are returned with ErrInvalidAPIEndpoint with the error string being invalid API endpoint.

This handler can be used with future/other routes by chi Mount'ing the handler to the specific route you want so that users are returned with the invalid endpoint error when trying to access any downstream route from the route/pattern the handler is chi Mount'ed to.

Tests were successful for the tested for this fix (test created by @nopcoder - thank you!), but I still had issues with getting TestLocalLoad to pass successfully. I wanted to put this PR up for review and then we can talk about any issues it has AND how to resolve the testing issue for TestLocalLoad.

Thank you.

@CLAassistant
Copy link

CLAassistant commented Aug 17, 2021

CLA assistant check
All committers have signed the CLA.

pkg/api/ui_handler.go Outdated Show resolved Hide resolved
pkg/api/ui_handler.go Outdated Show resolved Hide resolved
This commit works to close issue treeverse#2190 by creating handler that returns
ErrInvalidAPIEndpoint when called. This will be used by chi Mount'ing
the handler to a specified route so that any calls to a route downstream
the Mount'd pattern will respond with the ErrInvalidAPIEndpoint error.

Also add corresponding test, TestInvalidRoute, to test
InvalidAPIEndpointHandler.

Create middleware to catch downstream routes from BaseURL

This commit works to close issue treeverse#2190 by creating middleware to ensure
users that call routes downstream of the BaseURL (i.e. /api/v1) are
return an invalid endpoint error and internal error status.

Remove BaseURLHandler
@DataDavD DataDavD force-pushed the fix/api-unknown-path-return-error-2190 branch from 846a775 to c96a22d Compare August 19, 2021 10:13
@DataDavD
Copy link
Contributor Author

Hey @nopcoder thank you so much for the great suggestions/info (i.e. basically doing the work for me 🤣). Anyways, I made the changes needed. I tried to rebase my history so I don't have multiple commits just adding and removing stuff, but since my rebase experience is lacking I had a hard time.

FYI I think I got it right, but please let me know if its an issue. To be clear, so long as I rebase my forked commits, rebasing should be fine before I merge my PR branch, correct?

Thanks again for all your help.

best,
dd

@DataDavD
Copy link
Contributor Author

Hey @nopcoder FYI I forgot to mention that the tests were still successful after the change, but I still had the TestLocalLoad failing :(

@nopcoder
Copy link
Contributor

nopcoder commented Aug 20, 2021

@DataDavD all you - will squash and merge it after the tests will complete green on GitHub.
If you like me to help with debugging why the test doesn't run locally - you are welcome to jump on our slack and post the error. The team and myself would be happy to help.

@nopcoder nopcoder linked an issue Aug 20, 2021 that may be closed by this pull request
@nopcoder nopcoder merged commit d509894 into treeverse:master Aug 20, 2021
@DataDavD
Copy link
Contributor Author

DataDavD commented Aug 20, 2021

Ok yes, I will definitely do that, especially since I want to help contribute more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API with an unknown path should return an error
3 participants